From 842c182e67a3b380d0fd70b7d41d3755d6643977 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 1 Jun 2017 22:33:02 -0700 Subject: [PATCH] Optimize the interface of `Registry`. Previously all intermediate stages would create and return `Vec`, but this is a pretty costly operation once you start layering. Ideally we'd use an iterator-based approach here but working with that in trait objects is difficult, so this commit takes a closure-based approach to avoid all the intermediate allocations that are thrown away. --- src/cargo/core/registry.rs | 124 +++++++++++++++++++--------- src/cargo/core/resolver/mod.rs | 23 +++--- src/cargo/ops/cargo_install.rs | 2 +- src/cargo/sources/directory.rs | 10 ++- src/cargo/sources/git/source.rs | 6 +- src/cargo/sources/path.rs | 6 +- src/cargo/sources/registry/index.rs | 30 ++++--- src/cargo/sources/registry/mod.rs | 15 +++- src/cargo/sources/replaced.rs | 17 ++-- 9 files changed, 156 insertions(+), 77 deletions(-) diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index 04042b442..6c7962049 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -1,3 +1,4 @@ +use std::cell::RefCell; use std::collections::HashMap; use core::{Source, SourceId, SourceMap, Summary, Dependency, PackageId, Package}; @@ -11,7 +12,15 @@ use sources::config::SourceConfigMap; /// See also `core::Source`. pub trait Registry { /// Attempt to find the packages that match a dependency request. - fn query(&mut self, name: &Dependency) -> CargoResult>; + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()>; + + fn query_vec(&mut self, dep: &Dependency) -> CargoResult> { + let mut ret = Vec::new(); + self.query(dep, &mut |s| ret.push(s))?; + Ok(ret) + } /// Returns whether or not this registry will return summaries with /// checksums listed. @@ -23,22 +32,34 @@ pub trait Registry { } impl Registry for Vec { - fn query(&mut self, dep: &Dependency) -> CargoResult> { - Ok(self.iter().filter(|summary| dep.matches(*summary)) - .cloned().collect()) + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { + for summary in self.iter().filter(|summary| dep.matches(*summary)) { + f(summary.clone()); + } + Ok(()) } } impl Registry for Vec { - fn query(&mut self, dep: &Dependency) -> CargoResult> { - Ok(self.iter().filter(|pkg| dep.matches(pkg.summary())) - .map(|pkg| pkg.summary().clone()).collect()) + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { + for summary in self.iter() + .map(|p| p.summary()) + .filter(|summary| dep.matches(*summary)) { + f(summary.clone()); + } + Ok(()) } } impl<'a, T: ?Sized + Registry + 'a> Registry for Box { - fn query(&mut self, name: &Dependency) -> CargoResult> { - (**self).query(name) + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { + (**self).query(dep, f) } } @@ -56,7 +77,7 @@ impl<'a, T: ?Sized + Registry + 'a> Registry for Box { /// a `Source`. Each `Source` in the map has been updated (using network /// operations if necessary) and is ready to be queried for packages. pub struct PackageRegistry<'cfg> { - sources: SourceMap<'cfg>, + sources: RefCell>, // A list of sources which are considered "overrides" which take precedent // when querying for packages. @@ -94,7 +115,7 @@ impl<'cfg> PackageRegistry<'cfg> { pub fn new(config: &'cfg Config) -> CargoResult> { let source_config = SourceConfigMap::new(config)?; Ok(PackageRegistry { - sources: SourceMap::new(), + sources: RefCell::new(SourceMap::new()), source_ids: HashMap::new(), overrides: Vec::new(), source_config: source_config, @@ -103,8 +124,9 @@ impl<'cfg> PackageRegistry<'cfg> { } pub fn get(self, package_ids: &[PackageId]) -> PackageSet<'cfg> { - trace!("getting packages; sources={}", self.sources.len()); - PackageSet::new(package_ids, self.sources) + let sources = self.sources.into_inner(); + trace!("getting packages; sources={}", sources.len()); + PackageSet::new(package_ids, sources) } fn ensure_loaded(&mut self, namespace: &SourceId, kind: Kind) -> CargoResult<()> { @@ -156,7 +178,7 @@ impl<'cfg> PackageRegistry<'cfg> { fn add_source(&mut self, source: Box, kind: Kind) { let id = source.source_id().clone(); - self.sources.insert(source); + self.sources.get_mut().insert(source); self.source_ids.insert(id.clone(), (id, kind)); } @@ -189,16 +211,16 @@ impl<'cfg> PackageRegistry<'cfg> { // Ensure the source has fetched all necessary remote data. let _p = profile::start(format!("updating: {}", source_id)); - self.sources.get_mut(source_id).unwrap().update() + self.sources.get_mut().get_mut(source_id).unwrap().update() })().chain_err(|| format!("Unable to update {}", source_id)) } fn query_overrides(&mut self, dep: &Dependency) -> CargoResult> { for s in self.overrides.iter() { - let src = self.sources.get_mut(s).unwrap(); + let src = self.sources.get_mut().get_mut(s).unwrap(); let dep = Dependency::new_override(dep.name(), s); - let mut results = src.query(&dep)?; + let mut results = src.query_vec(&dep)?; if results.len() > 0 { return Ok(Some(results.remove(0))) } @@ -335,35 +357,58 @@ http://doc.crates.io/specifying-dependencies.html#overriding-dependencies } impl<'cfg> Registry for PackageRegistry<'cfg> { - fn query(&mut self, dep: &Dependency) -> CargoResult> { + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { // Ensure the requested source_id is loaded self.ensure_loaded(dep.source_id(), Kind::Normal).chain_err(|| { format!("failed to load source for a dependency \ on `{}`", dep.name()) })?; + // Look for an override and get ready to query the real source let override_summary = self.query_overrides(&dep)?; - let real_summaries = match self.sources.get_mut(dep.source_id()) { - Some(src) => Some(src.query(&dep)?), - None => None, + let mut sources = self.sources.borrow_mut(); + let source = match sources.get_mut(dep.source_id()) { + Some(src) => src, + None => { + if override_summary.is_some() { + bail!("override found but no real ones"); + } + return Ok(()) + } }; - let ret = match (override_summary, real_summaries) { - (Some(candidate), Some(summaries)) => { - if summaries.len() != 1 { - bail!("found an override with a non-locked list"); - } - self.warn_bad_override(&candidate, &summaries[0])?; - vec![candidate] + // Query the real source, keeping track of some extra info. If we've got + // an override we don't actually ship up summaries. If we do ship up + // summaries though we be sure to postprocess them to a locked version + // to assist with lockfiles and conservative updates. + let mut n = 0; + let mut to_warn = None; + source.query(dep, &mut |summary| { + n += 1; + if override_summary.is_none() { + f(self.lock(summary)) + } else { + to_warn = Some(summary); } - (Some(_), None) => bail!("override found but no real ones"), - (None, Some(summaries)) => summaries, - (None, None) => Vec::new(), + })?; + + // After all that's said and done we need to check the override summary + // itself. If present we'll do some more validation and yield it up, + // otherwise we're done. + let override_summary = match override_summary { + Some(s) => s, + None => return Ok(()) }; - // post-process all returned summaries to ensure that we lock all - // relevant summaries to the right versions and sources - Ok(ret.into_iter().map(|summary| self.lock(summary)).collect()) + if n > 1 { + bail!("found an override with a non-locked list"); + } else if let Some(summary) = to_warn { + self.warn_bad_override(&override_summary, &summary)?; + } + f(self.lock(override_summary)); + Ok(()) } } @@ -411,15 +456,20 @@ pub mod test { } impl Registry for RegistryBuilder { - fn query(&mut self, dep: &Dependency) -> CargoResult> { + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { debug!("querying; dep={:?}", dep); let overrides = self.query_overrides(dep); if overrides.is_empty() { - self.summaries.query(dep) + self.summaries.query(dep, f) } else { - Ok(overrides) + for s in overrides { + f(s); + } + Ok(()) } } } diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 29c464ae8..ecceb6456 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -718,7 +718,7 @@ fn activation_error(cx: &Context, let all_req = semver::VersionReq::parse("*").unwrap(); let mut new_dep = dep.clone(); new_dep.set_version_req(all_req); - let mut candidates = match registry.query(&new_dep) { + let mut candidates = match registry.query_vec(&new_dep) { Ok(candidates) => candidates, Err(e) => return e, }; @@ -949,21 +949,23 @@ impl<'a> Context<'a> { fn query(&self, registry: &mut Registry, dep: &Dependency) -> CargoResult> { - let summaries = registry.query(dep)?; - summaries.into_iter().map(|summary| { - // get around lack of non-lexical lifetimes - let summary2 = summary.clone(); + let mut ret = Vec::new(); + registry.query(dep, &mut |s| { + ret.push(Candidate { summary: s, replace: None }); + })?; + for candidate in ret.iter_mut() { + let summary = &candidate.summary; let mut potential_matches = self.replacements.iter() - .filter(|&&(ref spec, _)| spec.matches(summary2.package_id())); + .filter(|&&(ref spec, _)| spec.matches(summary.package_id())); let &(ref spec, ref dep) = match potential_matches.next() { - None => return Ok(Candidate { summary: summary, replace: None }), + None => continue, Some(replacement) => replacement, }; debug!("found an override for {} {}", dep.name(), dep.version_req()); - let mut summaries = registry.query(dep)?.into_iter(); + let mut summaries = registry.query_vec(dep)?.into_iter(); let s = summaries.next().ok_or_else(|| { format!("no matching package for override `{}` found\n\ location searched: {}\n\ @@ -1005,8 +1007,9 @@ impl<'a> Context<'a> { debug!("\t{} => {}", dep.name(), dep.version_req()); } - Ok(Candidate { summary: summary, replace: replace }) - }).collect() + candidate.replace = replace; + } + Ok(ret) } fn prev_active(&self, dep: &Dependency) -> &[Summary] { diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index ba1829acc..ceb67c323 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -295,7 +295,7 @@ fn select_pkg<'a, T>(mut source: T, }; let vers = vers.as_ref().map(|s| &**s); let dep = Dependency::parse_no_deprecated(name, vers, source.source_id())?; - let deps = source.query(&dep)?; + let deps = source.query_vec(&dep)?; match deps.iter().map(|p| p.package_id()).max() { Some(pkgid) => { let pkg = source.download(pkgid)?; diff --git a/src/cargo/sources/directory.rs b/src/cargo/sources/directory.rs index 45a74f1b6..d04613156 100644 --- a/src/cargo/sources/directory.rs +++ b/src/cargo/sources/directory.rs @@ -45,11 +45,15 @@ impl<'cfg> Debug for DirectorySource<'cfg> { } impl<'cfg> Registry for DirectorySource<'cfg> { - fn query(&mut self, dep: &Dependency) -> CargoResult> { + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { let packages = self.packages.values().map(|p| &p.0); let matches = packages.filter(|pkg| dep.matches(pkg.summary())); - let summaries = matches.map(|pkg| pkg.summary().clone()); - Ok(summaries.collect()) + for summary in matches.map(|pkg| pkg.summary().clone()) { + f(summary); + } + Ok(()) } fn supports_checksums(&self) -> bool { diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index 634c1814b..4a3f8f6f6 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -115,10 +115,12 @@ impl<'cfg> Debug for GitSource<'cfg> { } impl<'cfg> Registry for GitSource<'cfg> { - fn query(&mut self, dep: &Dependency) -> CargoResult> { + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { let src = self.path_source.as_mut() .expect("BUG: update() must be called before query()"); - src.query(dep) + src.query(dep, f) } } diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index dd27076cf..2f3c30141 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -317,8 +317,10 @@ impl<'cfg> Debug for PathSource<'cfg> { } impl<'cfg> Registry for PathSource<'cfg> { - fn query(&mut self, dep: &Dependency) -> CargoResult> { - self.packages.query(dep) + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { + self.packages.query(dep, f) } } diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index 036d23dda..b92f717b5 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -5,7 +5,7 @@ use std::str; use serde_json; use core::dependency::{Dependency, DependencyInner, Kind}; -use core::{SourceId, Summary, PackageId, Registry}; +use core::{SourceId, Summary, PackageId}; use sources::registry::{RegistryPackage, RegistryDependency, INDEX_LOCK}; use sources::registry::RegistryData; use util::{CargoError, CargoResult, internal, Filesystem, Config}; @@ -181,21 +181,21 @@ impl<'cfg> RegistryIndex<'cfg> { pub fn query(&mut self, dep: &Dependency, - load: &mut RegistryData) - -> CargoResult> { - let mut summaries = { - let summaries = self.summaries(dep.name(), load)?; - summaries.iter().filter(|&&(_, yanked)| { - dep.source_id().precise().is_some() || !yanked - }).map(|s| s.0.clone()).collect::>() - }; + load: &mut RegistryData, + f: &mut FnMut(Summary)) + -> CargoResult<()> { + let source_id = self.source_id.clone(); + let summaries = self.summaries(dep.name(), load)?; + let summaries = summaries.iter().filter(|&&(_, yanked)| { + dep.source_id().precise().is_some() || !yanked + }).map(|s| s.0.clone()); // Handle `cargo update --precise` here. If specified, our own source // will have a precise version listed of the form `=` where // `` is the name of a crate on this source and `` is the // version requested (agument to `--precise`). - summaries.retain(|s| { - match self.source_id.precise() { + let summaries = summaries.filter(|s| { + match source_id.precise() { Some(p) if p.starts_with(dep.name()) && p[dep.name().len()..].starts_with('=') => { let vers = &p[dep.name().len() + 1..]; @@ -204,6 +204,12 @@ impl<'cfg> RegistryIndex<'cfg> { _ => true, } }); - summaries.query(dep) + + for summary in summaries { + if dep.matches(&summary) { + f(summary); + } + } + Ok(()) } } diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index 139d8a607..66f267a79 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -320,18 +320,27 @@ impl<'cfg> RegistrySource<'cfg> { } impl<'cfg> Registry for RegistrySource<'cfg> { - fn query(&mut self, dep: &Dependency) -> CargoResult> { + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { // If this is a precise dependency, then it came from a lockfile and in // theory the registry is known to contain this version. If, however, we // come back with no summaries, then our registry may need to be // updated, so we fall back to performing a lazy update. if dep.source_id().precise().is_some() && !self.updated { - if self.index.query(dep, &mut *self.ops)?.is_empty() { + let mut called = false; + self.index.query(dep, &mut *self.ops, &mut |s| { + called = true; + f(s); + })?; + if called { + return Ok(()) + } else { self.do_update()?; } } - self.index.query(dep, &mut *self.ops) + self.index.query(dep, &mut *self.ops, f) } fn supports_checksums(&self) -> bool { diff --git a/src/cargo/sources/replaced.rs b/src/cargo/sources/replaced.rs index 47d9feb68..d2d65e542 100644 --- a/src/cargo/sources/replaced.rs +++ b/src/cargo/sources/replaced.rs @@ -20,15 +20,18 @@ impl<'cfg> ReplacedSource<'cfg> { } impl<'cfg> Registry for ReplacedSource<'cfg> { - fn query(&mut self, dep: &Dependency) -> CargoResult> { - let dep = dep.clone().map_source(&self.to_replace, &self.replace_with); - let ret = self.inner.query(&dep).chain_err(|| { + fn query(&mut self, + dep: &Dependency, + f: &mut FnMut(Summary)) -> CargoResult<()> { + let (replace_with, to_replace) = (&self.replace_with, &self.to_replace); + let dep = dep.clone().map_source(to_replace, replace_with); + + self.inner.query(&dep, &mut |summary| { + f(summary.map_source(replace_with, to_replace)) + }).chain_err(|| { format!("failed to query replaced source `{}`", self.to_replace) - })?; - Ok(ret.into_iter().map(|summary| { - summary.map_source(&self.replace_with, &self.to_replace) - }).collect()) + }) } } -- 2.30.2